BUG: fixed .str.contains(..., na=False) for categorical series #22170

pulkitmaloo · 2018-08-02T14:02:13Z

closes .str.contains(..., na=False) consistency between categorical and object series #22158
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

TomAugspurger · 2018-08-14T14:58:38Z

Failure is at https://travis-ci.org/pandas-dev/pandas/jobs/411345500#L2287-L2289

Basically, you're fix only works if there are missing values.

This may fix it, but may break other things

diff --git a/pandas/core/strings.py b/pandas/core/strings.py
index 8097ed196..06fd1391e 100644
--- a/pandas/core/strings.py
+++ b/pandas/core/strings.py
@@ -1855,7 +1855,12 @@ class StringMethods(NoNewAttributesMixin):
         # before the transformation...
         if use_codes and self._is_categorical:
             result = take_1d(result, self._orig.cat.codes)
-            result[isna(result)] = na
+            missing = isna(result)
+
+            if missing.any():
+                result_type = np.result_type(result, na)
+                result = result.astype(result_type, copy=False)
+                result[isna(result)] = na
 
         if not hasattr(result, 'ndim') or not hasattr(result, 'dtype'):
             return result

pep8speaks · 2018-08-17T18:58:08Z

Hello @pulkitmaloo! Thanks for updating the PR.

There are no PEP8 issues in the file pandas/core/strings.py !
There are no PEP8 issues in the file pandas/tests/test_strings.py !

Comment last updated on September 24, 2018 at 01:15 Hours UTC

TomAugspurger · 2018-08-18T19:16:58Z

Looks like a maybe unrelated CI failure. Can you merge master and push again?

TomAugspurger · 2018-08-18T19:21:15Z

And can you ensure (and test) that na is handled correctly for all the string functions? It looks like at least match isn't handled correctly.

…ategorical_bug

codecov · 2018-08-24T14:14:03Z

Codecov Report

❗ No coverage uploaded for pull request base (master@960a73f). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #22170   +/-   ##
=========================================
  Coverage          ?   92.19%           
=========================================
  Files             ?      169           
  Lines             ?    50950           
  Branches          ?        0           
=========================================
  Hits              ?    46975           
  Misses            ?     3975           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`90.62% <100%> (?)`
#single	`42.27% <0%> (?)`

Impacted Files	Coverage Δ
pandas/core/strings.py	`98.58% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 960a73f...44b36a4. Read the comment docs.

… into categorical_bug

jreback · 2018-09-18T13:27:24Z

ci/circle-27-compat.yaml

@@ -7,7 +7,7 @@ dependencies:
  - cython=0.28.2
  - jinja2=2.8
  - numexpr=2.4.4 # we test that we correctly don't use an unsupported numexpr
-  - numpy=1.9.2


can you revert this (and the rest of the .yaml)

pandas/core/strings.py

pandas/tests/test_strings.py

… into categorical_bug

pulkitmaloo · 2018-09-23T11:33:59Z

@jreback Thanks for the feedback, I have updated the PR as you requested. However, the changes in yaml files leads to circleci failure.

jreback · 2018-09-23T13:13:18Z

@pulkitmaloo you need to rebase on master

… into categorical_bug

jreback

looks good, can you add a whatsnew note in bug fixes

jreback · 2018-09-25T14:55:28Z

doc/source/whatsnew/v0.24.0.txt

@@ -704,7 +704,7 @@ Numeric
 Strings
 ^^^^^^^

-
+- Bug in :class:`StringMethods` where `str_contains()` was not filling missing values with given argument for na for a categorical Series (:issue:`22158`)


can you use double backticks on Series, can you use the actual reference here, e.g. :func:`Series.str.contains`

@jreback Thanks, I've updated it. Please let me know if there's anything else you'd like to change.

@jreback Can you please review and merge this PR.

doc/source/whatsnew/v0.24.0.txt

pandas/tests/test_strings.py

Co-Authored-By: pulkitmaloo <[email protected]>

TomAugspurger · 2018-11-02T17:21:47Z

pandas/tests/test_strings.py

+        values = Series(["a", "b", "c", "a", np.nan], dtype="category")
+        result = values.str.contains('a', na=True).astype(object)
+        expected = Series([True, False, False, True, True], dtype=np.object_)
+        assert isinstance(result, Series)


this can be removed. The assert_series_equal checks that.

TomAugspurger · 2018-11-02T17:23:42Z

pandas/tests/test_strings.py

-        assert res.loc[2] == "foo"
+        # na for category
+        values = Series(["a", "b", "c", "a", np.nan], dtype="category")
+        result = values.str.contains('a', na=True).astype(object)


Why the .astype(object). What's the dtype when you don't do that? Categorical? If so, I'd rather we document that as part of the API and test it.

TomAugspurger · 2018-11-02T17:24:04Z

pandas/tests/test_strings.py

+
+        # na for objects
+        values = Series(["a", "b", "c", "a", np.nan])
+        result = values.str.contains('a', na=True).astype(object)


Same comment for the astype object.

TomAugspurger · 2018-11-09T16:13:39Z

@pulkitmaloo are you able to update?

jreback · 2018-11-18T22:19:57Z

@TomAugspurger should be fixed up.

jreback · 2018-11-20T01:14:01Z

thanks @pulkitmaloo

…fixed * upstream/master: DOC: more consistent flake8-commands in contributing.rst (pandas-dev#23724) DOC: Fixed the doctsring for _set_axis_name (GH 22895) (pandas-dev#22969) DOC: Improve GL03 message re: blank lines at end of docstrings. (pandas-dev#23649) TST: add tests for keeping dtype in Series.update (pandas-dev#23604) TST: For GH4861, Period and datetime in multiindex (pandas-dev#23776) TST: move .str-test to strings.py & parametrize it; precursor to pandas-dev#23582 (pandas-dev#23777) STY: isort tests/scalar, tests/tslibs, import libwindow instead of _window (pandas-dev#23787) BUG: fixed .str.contains(..., na=False) for categorical series (pandas-dev#22170) BUG: Maintain column order with groupby.nth (pandas-dev#22811) API/DEPR: replace kwarg "pat" with "sep" in str.[r]partition (pandas-dev#23767) CLN: Finish isort core (pandas-dev#23765) TST: Mark test_pct_max_many_rows with pytest.mark.single (pandas-dev#23799)

…s-dev#22170)

pulkitmaloo added 2 commits August 2, 2018 09:48

BUG: fixed .str.contains(..., na=False) for categorical series

077d136

na argument for _wrap_results

2ae44d1

datapythonista added Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Strings String extension data type and string data Categorical Categorical Data Type labels Aug 2, 2018

fixed str.contains for missing values

a1b3d7b

PEP8 Issue: added whitespace after ','

dbd990b

Merge branch 'master' of https://github.com/pulkitmaloo/pandas into c…

9d5d2c2

…ategorical_bug

pulkitmaloo added 8 commits August 24, 2018 14:32

na argument for wrap_results in match

90aef7b

BUG: fixed .str.contains(..., na=False) for categorical series

69f16af

na argument for _wrap_results

78cf8c7

fixed str.contains for missing values

93bb24a

PEP8 Issue: added whitespace after ','

6c2700f

na argument for wrap_results in match

6649129

Merge branch 'categorical_bug' of https://github.com/pulkitmaloo/pandas…

5c87e81

… into categorical_bug

Merge branch 'master' into categorical_bug

a09dcc5

jreback requested changes Sep 18, 2018

View reviewed changes

pandas/tests/test_strings.py Outdated Show resolved Hide resolved

pulkitmaloo added 5 commits September 22, 2018 15:27

Update circle-27-compat.yaml

3abdea5

Update travis-27-locale.yaml

d136599

Merge branch 'categorical_bug' of https://github.com/pulkitmaloo/pandas…

b942e16

… into categorical_bug

added tests for na arg for categorical and objects

82f9b9e

updated _wrap_results with arg fill_value and removed na

53e9253

pulkitmaloo added 7 commits September 23, 2018 20:52

na argument for wrap_results in match

1f0256a

Update travis-27-locale.yaml

7025f34

added tests for na arg for categorical and objects

7542448

updated _wrap_results with arg fill_value and removed na

f1b4274

Merge branch 'categorical_bug' of https://github.com/pulkitmaloo/pandas…

386ab98

… into categorical_bug

Update travis-27-locale.yaml

7a09c44

fixed line too long

3408920

jreback added this to the 0.24.0 milestone Sep 25, 2018

jreback requested changes Sep 25, 2018

View reviewed changes

whatsnew note

3288d11

jreback requested changes Sep 25, 2018

View reviewed changes

Update v0.24.0.txt

6c87770

TomAugspurger reviewed Oct 18, 2018

View reviewed changes

doc/source/whatsnew/v0.24.0.txt Outdated Show resolved Hide resolved

jreback requested changes Oct 18, 2018

View reviewed changes

pandas/tests/test_strings.py Show resolved Hide resolved

Update doc/source/whatsnew/v0.24.0.txt

d242647

Co-Authored-By: pulkitmaloo <[email protected]>

TomAugspurger reviewed Nov 2, 2018

View reviewed changes

TomAugspurger mentioned this pull request Nov 9, 2018

Unexpected behavior with NaN values on str operators with categorical data #23602

Closed

jreback added 3 commits November 18, 2018 17:10

Merge branch 'master' into PR_TOOL_MERGE_PR_22170

def1b4e

whatsnew

fd99431

cleanup

44b36a4

jreback approved these changes Nov 18, 2018

View reviewed changes

jreback merged commit 2af56d4 into pandas-dev:master Nov 20, 2018

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG: fixed .str.contains(..., na=False) for categorical series (panda…

4072ed5

…s-dev#22170)

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG: fixed .str.contains(..., na=False) for categorical series (panda…

ebbca06

…s-dev#22170)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: fixed .str.contains(..., na=False) for categorical series #22170

BUG: fixed .str.contains(..., na=False) for categorical series #22170

pulkitmaloo commented Aug 2, 2018 •

edited

Loading

TomAugspurger commented Aug 14, 2018

pep8speaks commented Aug 17, 2018 •

edited

Loading

TomAugspurger commented Aug 18, 2018

TomAugspurger commented Aug 18, 2018

codecov bot commented Aug 24, 2018 •

edited

Loading

jreback Sep 18, 2018

pulkitmaloo commented Sep 23, 2018

jreback commented Sep 23, 2018

jreback left a comment

jreback Sep 25, 2018

pulkitmaloo Sep 26, 2018 •

edited

Loading

pulkitmaloo Oct 18, 2018 •

edited

Loading

TomAugspurger Nov 2, 2018

TomAugspurger Nov 2, 2018

TomAugspurger Nov 2, 2018

TomAugspurger commented Nov 9, 2018

jreback commented Nov 18, 2018

jreback commented Nov 20, 2018

BUG: fixed .str.contains(..., na=False) for categorical series #22170

BUG: fixed .str.contains(..., na=False) for categorical series #22170

Conversation

pulkitmaloo commented Aug 2, 2018 • edited Loading

TomAugspurger commented Aug 14, 2018

pep8speaks commented Aug 17, 2018 • edited Loading

Comment last updated on September 24, 2018 at 01:15 Hours UTC

TomAugspurger commented Aug 18, 2018

TomAugspurger commented Aug 18, 2018

codecov bot commented Aug 24, 2018 • edited Loading

Codecov Report

jreback Sep 18, 2018

Choose a reason for hiding this comment

pulkitmaloo commented Sep 23, 2018

jreback commented Sep 23, 2018

jreback left a comment

Choose a reason for hiding this comment

jreback Sep 25, 2018

Choose a reason for hiding this comment

pulkitmaloo Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

pulkitmaloo Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

TomAugspurger Nov 2, 2018

Choose a reason for hiding this comment

TomAugspurger Nov 2, 2018

Choose a reason for hiding this comment

TomAugspurger Nov 2, 2018

Choose a reason for hiding this comment

TomAugspurger commented Nov 9, 2018

jreback commented Nov 18, 2018

jreback commented Nov 20, 2018

pulkitmaloo commented Aug 2, 2018 •

edited

Loading

pep8speaks commented Aug 17, 2018 •

edited

Loading

codecov bot commented Aug 24, 2018 •

edited

Loading

pulkitmaloo Sep 26, 2018 •

edited

Loading

pulkitmaloo Oct 18, 2018 •

edited

Loading